Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comment Detail: Fix cell separators not aligning correctly after rotation #17451

Merged
merged 1 commit into from
Nov 12, 2021

Conversation

dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Nov 11, 2021

Refs #17087

As titled, this PR fixes the issue where sometimes the cell separator is still displaying the margins for the landscape orientation (around 67pt from the view edge) when the screen is already displayed in portrait (where it should be around 20pt). Here's a screenshot:

Before After
Content alignment, portrait.1 before_noDelete_portrait after_noDelete_portrait
Content alignment, landscape.2 before_noDelete_landscape after_noDelete_landscape
Content alignment (with Delete) button, portrait.3 before_withDelete_portrait after_withDelete_portrait
Content alignment (with Delete) button, landscape.4 before_withDelete_landscape after_withDelete_landscape

To Test

  • Enable newCommentDetail feature flag.
  • Navigate to My Site > Comments, and select any approved comments.
  • 🔎 Verify that the cells' contents are neatly aligned.1
  • Rotate the device/simulator to landscape.
  • 🔎 Verify that the cell separators are aligned correctly.
  • Rotate the device/simulator back to portrait.
  • 🔎 Again, verify that the cell separators are aligned correctly.
  • Go back to My Site > Comments, and select any spam or trashed comments. Repeat the verification steps above.
  • 🔎 Also verify that while in portrait or landscape, the cells before the Delete button does not show a separator line.

Regression Notes

  1. Potential unintended areas of impact
    n/a. Feature is not released yet.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manually tested to ensure that it behaves correctly.

  3. What automated tests I added (or what prevented me from doing so)

  4. n/a. Feature is not released yet.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Footnotes

  1. Notice in the before image the content cell's right side is not aligned with the others, making it look asymmetrical since the right side is closer to the trailing edge. 2

  2. Some of the cell separators overextended to the left side of the cells.

  3. Some cells have incorrect content insets / indentation level. This is due to the cell still using the margins in the landscape mode.

  4. Some of the cell separators overextended to the left side of the cells.

@peril-wordpress-mobile
Copy link

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@ScoutHarris ScoutHarris self-requested a review November 11, 2021 17:13
Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

:shipit:

@dvdchr
Copy link
Contributor Author

dvdchr commented Nov 12, 2021

Thanks @ScoutHarris !

@dvdchr dvdchr merged commit b15d0d9 into develop Nov 12, 2021
@dvdchr dvdchr deleted the issue/17087-comment-detail-separator branch November 12, 2021 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants